Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTP status code to APIException #19

Closed
wants to merge 1 commit into from

Conversation

Acidity
Copy link

@Acidity Acidity commented Jan 2, 2016

No description provided.

@coveralls
Copy link

coveralls commented May 7, 2016

Coverage Status

Coverage increased (+0.02%) to 96.981% when pulling 3656b82 on Acidity:feature/error_status_codes into a5e5375 on Dreae:master.

@wtfrank
Copy link
Contributor

wtfrank commented Jul 25, 2016

OK so with the HTTP methods that CCP have started using, there's at least one example where a POST call returns status 201 instead of the more usual 200. Do we also need a method of communicating the return value after a successful call? Or do we only care about it in the failure case? In either case this pull is definitely useful.

@jonobrien
Copy link
Contributor

jonobrien commented Jul 25, 2016

Would be useful for debugging if we knew about return codes, but probably just for failure case currently?

I know there are different messages in the response body for invalid scope vs invalid callback uri that both return 401. Sometimes parsing the body on an error code could be useful too, but as far as return code, unless you've found it necessary on successful calls, probably not too important.

@hkraal
Copy link
Contributor

hkraal commented Jul 25, 2016

There is no reason to hide the actual response code from the user, if ones wants to it should be accessible. That's however something which should be covered with #7 along other possible return values.

In case of exceptions I really would like the HTTP code and content (if any) which is being returned if possible the URL + parameters could prove helpful as well. The Fittings endpoint has some special error messages which should be getting to the user.

@jonobrien
Copy link
Contributor

Yes I saw that Fittings error as well, good point. 🍰

@wtfrank
Copy link
Contributor

wtfrank commented Jul 25, 2016

So the exception needs to be augmented with any response text as well as just the response code.

I wonder if there is an exception in the requests library already? or maybe its better to have a pycrest specific exception class that does just what pycrest needs

regarding URL and parameters - you surely know what they are as you just made the call that is throwing an exception? Unless you mean for debugging, in which case maybe there's a case for using the python logging facility.

@hkraal
Copy link
Contributor

hkraal commented Aug 18, 2016

Closing as the improved version in #41 is now merged into master

@hkraal hkraal closed this Aug 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants